This would fix a lot of errors, like generator star spacing, space before () for anonymous functions, missing semi-colon, extra semi-colon, ... This would save a lot of time for people cleaning up the directory in the future. Patrick, thoughts ?

I use the --fix parameter systematically because it saves lots of time, and doesn't cause problems.

I had to split the patch in 3 parts, because the original was too big for bugzilla.

(In reply to Tim Nguyen :ntim from comment #0) > This would fix a lot of errors, like generator star spacing, space before () > for anonymous functions, missing semi-colon, extra semi-colon, ... > > This would save a lot of time for people cleaning up the directory in the > future. > > Patrick, thoughts ? Sounds like a great idea, but I don't want to have to review this. Oops, you already flagged me for review :) Don't think I've seen multi-MB patches on bugzilla before. I think I'm going to have to avoid splinter altogether and find another to quickly review this. Can you please list the rules that --fix works on? If it's just spacing and semi-colons, I don't think I should review the code changes at all, and just R+ this.

(In reply to Patrick Brosset <:pbro> from comment #5) > Sounds like a great idea, but I don't want to have to review this. Oops, you > already flagged me for review :) No you didn't. Sorry I got confused by another review request I had received.

I have seen auto-fixing occasionally produce something suboptimal with spacing and line breaking, since the rules can't fully describing everything, and auto-fix only knows "do something to satisfy the rules I know about". But still, those cases are rare... we could still choose to just rubber stamp the changes, and manually fix up edge cases if we notice them later. Or someone can review the giant thing like usual. For me, trying to load even these smaller patches in Splinter makes the browser hang... If we want to review, we may need either MozReview (since it has pagination), or download the patch file for local inspection and leave any notes as just bug comments.

It might be nice to be more fine-grained about this. Run it specifically on each tool instead of the entire `client` directory. Would make it easier to at least scan.

I would be happy to review this, but it would be nice to have it in an easy to review form. I would prefer MozReview myself, which should be easier to scan since it has pagination for long commits...? But breaking it up per client directory could work as well.

I'll take this on, so we can have a consistent style in place. :tromey has offered to review.

Let's see if MozReview can deal with one giant combined patch...

Review commit: https://reviewboard.mozilla.org/r/53282/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53282/

For simple rules like function spacing, we can auto-fix these across the code base so they are followed in a consistent way. To generate this patch, I ran: ./mach eslint devtools --no-ignore --fix After this, I reverted any changes to third party files that we really do want to ignore. Review commit: https://reviewboard.mozilla.org/r/53280/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53280/

Comment on attachment 8753457 [details] MozReview Request: Bug 1271084 - Remove missing files from .eslintignore. r=tromey https://reviewboard.mozilla.org/r/53282/#review50086 Thank you.

Comment on attachment 8753458 [details] MozReview Request: Bug 1271084 - Apply ESLint autofixes to ignored /devtools files. r=tromey https://reviewboard.mozilla.org/r/53280/#review50096 Thank you for this patch. I agree with the idea behind this patch, so r+. However there are a few issues I found while skimming the patch at high speed. I think at least these need to be addressed; ideally with some way to assure ourselves that we haven't missed similar issues. ::: devtools/client/debugger/test/mochitest/code_math.min.js:2 (Diff revision 1) > -function add(a,b,k){var result=a+b;return k(result)}function sub(a,b,k){var result=a-b;return k(result)}function mul(a,b,k){var result=a*b;return k(result)}function div(a,b,k){var result=a/b;return k(result)}function arithmetic(){add(4,4,function(a){sub(a,2,function(b){mul(b,3,function(c){div(c,2,function(d){console.log(d)})})})})}; > -//@ sourceMappingURL=code_math.map > +function add(a, b, k) {var result = a + b; return k(result);} function sub(a, b, k) {var result = a - b; return k(result);} function mul(a, b, k) {var result = a * b; return k(result);} function div(a, b, k) {var result = a / b; return k(result);} function arithmetic() {add(4, 4, function (a) {sub(a, 2, function (b) {mul(b, 3, function (c) {div(c, 2, function (d) {console.log(d);});});});});} > +// @ sourceMappingURL=code_math.map I suspect it's wrong to modify any file that has a source map. ::: devtools/client/debugger/test/mochitest/code_math_bogus_map.js:4 (Diff revision 1) > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > -function stopMe(){throw Error("boom");}try{stopMe();var a=1;a=a*2;}catch(e){}; > -//# sourceMappingURL=bogus.map > +function stopMe() {throw Error("boom");} try {stopMe(); var a = 1; a = a * 2;} catch (e) {} > +// # sourceMappingURL=bogus.map Source map. ::: devtools/client/debugger/test/mochitest/code_ugly-4.js:2 (Diff revision 1) > -function a(){b()}function b(){debugger} > -//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiYWJjLmpzIiwic291cmNlcyI6WyJkYXRhOnRleHQvamF2YXNjcmlwdCxmdW5jdGlvbiBhKCl7YigpfSIsImRhdGE6dGV4dC9qYXZhc2NyaXB0LGZ1bmN0aW9uIGIoKXtkZWJ1Z2dlcn0iXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUEsaUJDQUEsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMifQ== > +function a() {b();} function b() {debugger;} > +// # sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiYWJjLmpzIiwic291cmNlcyI6WyJkYXRhOnRleHQvamF2YXNjcmlwdCxmdW5jdGlvbiBhKCl7YigpfSIsImRhdGE6dGV4dC9qYXZhc2NyaXB0LGZ1bmN0aW9uIGIoKXtkZWJ1Z2dlcn0iXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUEsaUJDQUEsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMifQ== Source map. ::: devtools/client/debugger/test/mochitest/code_worker-source-map.js:2 (Diff revision 1) > // Generated by CoffeeScript 1.10.0 > -(function() { > +(function () { Generated code. ::: devtools/client/framework/test/code_ugly.js:1 (Diff revision 1) > -function foo() { var a=1; var b=2; bar(a, b); } > +function foo() { var a = 1; var b = 2; bar(a, b); } Maybe none of the files with "ugly" in their name should be touched. ::: devtools/client/jsonview/lib/require.js:3 (Diff revision 1) > /** vim: et:ts=4:sw=4:sts=4 > * @license RequireJS 2.1.15 Copyright (c) 2010-2014, The Dojo Foundation All Rights Reserved. > * Available via the MIT or new BSD license. Maybe an imported file so shouldn't be touched? ::: devtools/client/shared/demangle.js:6 (Diff revision 1) > /** > * Exposes a function that demangles function names. > * Can be found at: https://github.com/kripken/cxx_demangle > */ > -var demangle = (function() { > +var demangle = (function () { > // In Firefox CommonJS environment, the module boilerplate thinks it's node, As mentioned on irc, a generated file. ::: devtools/server/actors/utils/automation-timeline.js:7 (Diff revision 1) > * web-audio-automation-timeline - 1.0.3 > * https://github.com/jsantell/web-audio-automation-timeline > * MIT License, copyright (c) 2014 Jordan Santell > */ > -!function(e){if("object"==typeof exports&&"undefined"!=typeof module)module.exports=e();else if("function"==typeof define&&define.amd)define([],e);else{var f;"undefined"!=typeof window?f=window:"undefined"!=typeof global?f=global:"undefined"!=typeof self&&(f=self),f.Timeline=e()}}(function(){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ > +!function (e) {if ("object" == typeof exports && "undefined" != typeof module)module.exports = e(); else if ("function" == typeof define && define.amd)define([], e); else {var f; "undefined" != typeof window ? f = window : "undefined" != typeof global ? f = global : "undefined" != typeof self && (f = self), f.Timeline = e();}}(function () {var define, module, exports; return (function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports:{}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1:[function (require, module, exports) { > -module.exports = require("./lib/timeline").Timeline; > + module.exports = require("./lib/timeline").Timeline; Not clear if this is a file we should be modifying. ::: devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js:1 (Diff revision 1) > -(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ > +(function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports:{}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1:[function (require, module, exports) { Probably shouldn't touch this file.

https://reviewboard.mozilla.org/r/53280/#review50096 > I suspect it's wrong to modify any file that has a source map. Thanks for noticing these! I searched the set of changed files to find any that contain `sourceMappingURL` (anywhere in the file) and examined each one. I'll add the bad ones to an explicit ignore list. > Maybe an imported file so shouldn't be touched? Reverted, added to the ignore list. > As mentioned on irc, a generated file. Reverted, added to the ignore list. > Not clear if this is a file we should be modifying. Reverted, added to ignore list.

Review commit: https://reviewboard.mozilla.org/r/53356/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53356/

Comment on attachment 8753458 [details] MozReview Request: Bug 1271084 - Apply ESLint autofixes to ignored /devtools files. r=tromey Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53280/diff/1-2/

Comment on attachment 8753577 [details] MozReview Request: Bug 1271084 - Explicitly ignore files with specific style. r=tromey https://reviewboard.mozilla.org/r/53356/#review50282 Thank you. Looks great!

https://hg.mozilla.org/integration/fx-team/rev/d86d541ce0f3 https://hg.mozilla.org/integration/fx-team/rev/c4b1eb619c41 https://hg.mozilla.org/integration/fx-team/rev/9661652c2140

https://hg.mozilla.org/mozilla-central/rev/d86d541ce0f3 https://hg.mozilla.org/mozilla-central/rev/c4b1eb619c41 https://hg.mozilla.org/mozilla-central/rev/9661652c2140